Optionally disable logging in the data sampler to support predict_step #10127
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jstjohn
force-pushed
the
jstjohn/optional_logging_for_predict
branch
from
baf1c1f
to
c583c91
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| raise ValueError("non-None value not found") | ||
|
|
||
|
|
||
| def get_dtype_device(torch_object) -> Tuple[torch.dtype, torch.device]: # noqa: D103 |
Check notice
Code scanning / CodeQL
Explicit returns mixed with implicit (fall through) returns Note test
|
|
||
|
|
||
| # NOTE(SKH): These types are all wrong, but are close. The inner type must always be a torch.Tensor, but the outer container should be generic. | ||
| def batch_collator(batches: Optional[Union[Tuple[ReductionT], List[ReductionT]]]) -> Optional[ReductionT]: |
Check notice
Code scanning / CodeQL
Explicit returns mixed with implicit (fall through) returns Note test
| case [list(), *_]: | ||
| return [batch_collator([batch[i] for batch in batches]) for i in range(len(batches[0]))] | ||
| case None: | ||
| return None |
Check warning
Code scanning / CodeQL
Unreachable code Warning test
jstjohn
force-pushed
the
jstjohn/optional_logging_for_predict
branch
2 times, most recently
from
f278078
to
479e2d8
Compare
|
stopping this for now until our CI is out of maintenance |
jstjohn
force-pushed
the
jstjohn/optional_logging_for_predict
branch
from
ac2075b
to
b62a392
Compare
jstjohn
commented
Aug 15, 2024
jstjohn
force-pushed
the
jstjohn/optional_logging_for_predict
branch
from
eddcbb4
to
6f4e2d9
Compare
jstjohn
force-pushed
the
jstjohn/optional_logging_for_predict
branch
from
6f4e2d9
to
5766f47
Compare
jstjohn
force-pushed
the
jstjohn/optional_logging_for_predict
branch
from
e2672ee
to
88d9df9
Compare
jstjohn
force-pushed
the
jstjohn/optional_logging_for_predict
branch
from
88d9df9
to
92d6307
Compare
jstjohn
force-pushed
the
jstjohn/optional_logging_for_predict
branch
from
d0258da
to
0da36a9
Compare
|
Looks good, thanks! Can we clean up some of the old comments in the test script? |
Signed-off-by: John St John <jstjohn@nvidia.com>
Signed-off-by: John St John <jstjohn@nvidia.com>
Signed-off-by: John St John <jstjohn@nvidia.com>
Signed-off-by: John St John <jstjohn@nvidia.com>
Signed-off-by: John St John <jstjohn@nvidia.com>
…s to cwd Signed-off-by: John St John <jstjohn@nvidia.com>
Signed-off-by: John St John <jstjohn@nvidia.com>
Signed-off-by: John St John <jstjohn@nvidia.com>
Signed-off-by: John St John <jstjohn@nvidia.com>
Signed-off-by: John St John <jstjohn@nvidia.com>
Signed-off-by: John St John <jstjohn@nvidia.com>
jstjohn
force-pushed
the
jstjohn/optional_logging_for_predict
branch
from
236f257
to
a6ff157
Compare
WoodieDudy
pushed a commit
to WoodieDudy/NeMo
that referenced
this pull request
Aug 26, 2024
NVIDIA#10127) * Resolve merge conflicts with consumed sample logging Signed-off-by: John St John <jstjohn@nvidia.com> * Add test file that captures the predict step error Signed-off-by: John St John <jstjohn@nvidia.com> * Add fixme comment around proper checkpoint nemo2 handling Signed-off-by: John St John <jstjohn@nvidia.com> * Skip megatron training test on CPU nodes Signed-off-by: John St John <jstjohn@nvidia.com> * Move output_log to last arg for compatibility Signed-off-by: John St John <jstjohn@nvidia.com> * try setting the default root dir in predict to avoid writing artifacts to cwd Signed-off-by: John St John <jstjohn@nvidia.com> * Handle the new check for batch samplers to enable predict_step Signed-off-by: John St John <jstjohn@nvidia.com> * Only reset the global microbatch, not entire parallel state Signed-off-by: John St John <jstjohn@nvidia.com> * Destroy the right sets of state in test of lightning trainer Signed-off-by: John St John <jstjohn@nvidia.com> * Fix typo and rename state resetting functions Signed-off-by: John St John <jstjohn@nvidia.com> * Run test in a subprocess to avoid contaminating global state Signed-off-by: John St John <jstjohn@nvidia.com> --------- Signed-off-by: John St John <jstjohn@nvidia.com>
hemildesai
pushed a commit
that referenced
this pull request
Aug 28, 2024
#10127) * Resolve merge conflicts with consumed sample logging Signed-off-by: John St John <jstjohn@nvidia.com> * Add test file that captures the predict step error Signed-off-by: John St John <jstjohn@nvidia.com> * Add fixme comment around proper checkpoint nemo2 handling Signed-off-by: John St John <jstjohn@nvidia.com> * Skip megatron training test on CPU nodes Signed-off-by: John St John <jstjohn@nvidia.com> * Move output_log to last arg for compatibility Signed-off-by: John St John <jstjohn@nvidia.com> * try setting the default root dir in predict to avoid writing artifacts to cwd Signed-off-by: John St John <jstjohn@nvidia.com> * Handle the new check for batch samplers to enable predict_step Signed-off-by: John St John <jstjohn@nvidia.com> * Only reset the global microbatch, not entire parallel state Signed-off-by: John St John <jstjohn@nvidia.com> * Destroy the right sets of state in test of lightning trainer Signed-off-by: John St John <jstjohn@nvidia.com> * Fix typo and rename state resetting functions Signed-off-by: John St John <jstjohn@nvidia.com> * Run test in a subprocess to avoid contaminating global state Signed-off-by: John St John <jstjohn@nvidia.com> --------- Signed-off-by: John St John <jstjohn@nvidia.com>
adityavavre
pushed a commit
to adityavavre/NeMo
that referenced
this pull request
Sep 15, 2024
NVIDIA#10127) * Resolve merge conflicts with consumed sample logging Signed-off-by: John St John <jstjohn@nvidia.com> * Add test file that captures the predict step error Signed-off-by: John St John <jstjohn@nvidia.com> * Add fixme comment around proper checkpoint nemo2 handling Signed-off-by: John St John <jstjohn@nvidia.com> * Skip megatron training test on CPU nodes Signed-off-by: John St John <jstjohn@nvidia.com> * Move output_log to last arg for compatibility Signed-off-by: John St John <jstjohn@nvidia.com> * try setting the default root dir in predict to avoid writing artifacts to cwd Signed-off-by: John St John <jstjohn@nvidia.com> * Handle the new check for batch samplers to enable predict_step Signed-off-by: John St John <jstjohn@nvidia.com> * Only reset the global microbatch, not entire parallel state Signed-off-by: John St John <jstjohn@nvidia.com> * Destroy the right sets of state in test of lightning trainer Signed-off-by: John St John <jstjohn@nvidia.com> * Fix typo and rename state resetting functions Signed-off-by: John St John <jstjohn@nvidia.com> * Run test in a subprocess to avoid contaminating global state Signed-off-by: John St John <jstjohn@nvidia.com> --------- Signed-off-by: John St John <jstjohn@nvidia.com> Signed-off-by: adityavavre <aditya.vavre@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pytorch lightning's
predict_stepdoes not support logging on the module. This PR adds an option to the data sampler to disable logging which allows the predict step to work.